Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implementation of the datetime (with up to nanoseconds) has been added #1093

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

avpalienko
Copy link

No description provided.

Copy link
Member

@vadz vadz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, but I couldn't finish reviewing the changes, partially because there are just too many of them clearly unrelated to the topic of this PR and I got discouraged when I saw that I covered barely half. It would be really great if you could please leave just the datetime-related parts.

I also still think that we should handle datetime as strings for the backends that don't support them natively, as otherwise this new type doesn't seem very useful.

We also need at least some documentation for the new type, especially if it's only supported in Oracle and PostgreSQL backends (which is not obvious).

@@ -15,6 +15,8 @@
#include <map>
#include <string>
#include <sstream>
#include <chrono>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These headers are not used, should be removed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change to this file doesn't seem related to the PR topic.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change to this file doesn't seem related to the PR topic.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change to this file doesn't seem related to the PR topic.

Comment on lines +82 to +85
#ifndef NOMINMAX
# define NOMINMAX
#endif

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change doesn't seem related to the PR topic.

Comment on lines +108 to +110

default:
throw soci_error("Use element used with non-supported type.");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change doesn't seem related to the PR topic.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do want to list all the enum elements explicitly instead of using default in order to get -Wshadow, so x_datetime should be added to the case statements here instead of removing them.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes to this file don't seem related to the PR topic.

Comment on lines +199 to +203
os << "<vector>,";
if ( ind_ )
os << ind_->size ();
else
os << "null";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change doesn't seem related to the PR topic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants